-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Reduce opinionation of editorconfig #234
fix: Reduce opinionation of editorconfig #234
Conversation
|
||
## Operator whitespace | ||
# space_around_math_operator = true | ||
# space_after_comma = true | ||
# space_after_comma_in_for_statement = true | ||
# space_around_concat_operator = true | ||
# space_around_logical_operator = true | ||
# space_around_assign_operator = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting these out makes no sense?
Actually disregard since I misread these two diff lines entirely, they are just introducing the default values I missed. Teaches me to read code while sleep deprived.
@@ -50,17 +53,19 @@ align_array_table = false | |||
## Other indentation settings | |||
never_indent_before_if_condition = false | |||
never_indent_comment_on_if_branch = false | |||
keep_indents_on_empty_lines = false | |||
allow_non_indented_comments = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the usecase for non-indented comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular use case in mind except I see no reason why someone shouldn't be allowed to do it if they want to.
This stuff is all forced on anyone who has auto-formatting enabled, so I think we should only turn a setting on if we want to force all code to conform to it.
In other words, the question isn't "what's the use case" but rather "why is this so bad that no human being should ever be permitted to do it?"
line_space_after_local_or_assign_statement = max(2) | ||
line_space_after_function_statement = max(2) | ||
line_space_after_expression_statement = keep | ||
line_space_after_comment = keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several of these don't make sense, but particularly the function spacing.
Edit: Actually there seems to be a bug with the formatter where it's subtracting 1 from every specified line spacing. So 2 actually means 1. Highly confusing but that means the values of 2 here are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since code formatting is a matter of opinion, I don't think it's a question of whether it makes sense, which is more of a factual question, but rather whether it improves readability.
In this case, I was saving files under the original editorconfig and it was stripping out so much whitespace that all the code was running together as a single block, which for me makes it way harder to read.
I simply chose the less opinionated option here, so if someone really wants to format code in close blocks by hand they can, but at least the autoformatter won't force it on you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the original values using 1
broke because the EmmyLua behaviour is unfortunately not intuitive, but is sadly considered not-a-bug (see my edit): CppCXY/EmmyLuaCodeStyle#191 (comment)
So most of it in the updated version is actually fine, but the keep
values don't really make sense since that allows an infinite amount of empty lines to be added in some cases when it should be stripped down to 1, so everything should probably have max(2)
.
@@ -69,4 +74,4 @@ break_all_list_when_line_exceed = true | |||
## "Preferences" | |||
ignore_space_after_colon = false | |||
# remove_call_expression_list_finish_comma = false | |||
end_statement_with_semicolon = replace_with_newline | |||
end_statement_with_semicolon = keep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semicolons at the end of lines is a legacy some C(++) programmers bring to Lua, and is not idiomatic, nor should it be allowed in clean Lua code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just chose the less opinionated option again. I think occasionally (rarely, i'll grant) it is useful to put two statements on the same line for readability or grouping reasons.
Not particular committed to this though, there is nothing i've seen in the code base where this is used/applicable so I don't mind changing it back.
Additionally, none of the changes introduced any kind of minification, if any of it resulted in minified Lua code then that's a bug in the formatter you're using. (In fact: Some of the added options would have reduced some minification-like structures in existing code.) |
Addresses some comments in mamoniot#234
The new editorconfig was a little too strict when combined with automatic code formatting in VSCode, resulting in very undesirable minification/whitespace removal. I loosened it a bit. After some testing, I believe submitting autoformatted code should be quite safe after this change.